feat(helm): add HStore deployment chart - #3218
Open
bitflicker64 wants to merge 67 commits into
Open
bitflicker64 wants to merge 67 commits into
bitflicker64 wants to merge 67 commits into
Conversation
…tore disabled The Server wrapper now writes auth.admin_pa from the auth Secret alongside usePD and pd.peers, so an auth-enabled release keeps its configured admin password with init_store.enabled=false instead of silently falling back to the public default. The Secret value is rejected when it contains properties-parser metacharacters that would inject config lines or store a different password than the Secret holds. The new hubble component deploys the Hubble UI as a single-replica Deployment with pd and direct wiring modes, optional Ingress and H2 persistence, schema validation, render-time guards, docs, and CI coverage. PD-meta installs (auth enabled, or Hubble in pd mode) also announce the Server client Service URL to PD via server.urls_to_pd and server.deploy_in_k8s so discovery clients receive a resolvable address instead of the 0.0.0.0 default, and the Hubble wrapper writes server.host so current images bind all interfaces. Because current Hubble images authenticate their login against the cluster, rendering Hubble without server.auth fails unless explicitly overridden. The CI invalid-value step now fails on every case rather than only its last line, and positive renders cover both Hubble modes. Validated against a composition of master 1716c77 plus the current heads of apache#3119 (edf07d0), apache#3126 (b40c42f), and apache#3130 (198de19): fresh auth-enabled installs reach Ready with zero restarts, the admin credential comes from the Secret while unauthenticated and default-password requests get 401, and Hubble logs in with the Secret credential and reads cluster metadata through PD discovery, with its H2 metadata persisted on the PVC.
…xtraEnv overrides A PD PodDisruptionBudget with minAvailable below floor(replicas/2)+1 permits voluntary evictions that leave PD without a Raft majority, so the render now requires the quorum floor in addition to the existing blocks-all-drains upper bound. With 2 replicas no valid budget exists; the README documents the even/odd contract. extraEnv entries render after the chart-owned variables and Kubernetes lets the last duplicate win, so a duplicate name could silently override a validated contract such as HG_SERVER_INIT_STORE_ENABLED=false. Each component's extraEnv now rejects its chart-managed variable names. Boundary and negative render cases for both rules are part of the CI invalid-value step.
The run failed at startup because azure/setup-helm@v4 is not on the ASF-approved actions allowlist; install the pinned helm release from the official tarball in a plain run step instead.
…cheduling defaults Fresh installs now seed PD with partition.default-shard-count derived from the Store count (3 when store.replicas is at least 3, otherwise 1, matching PD's odd-only constraint and its 2-to-1 clamp), so a default 3-Store deployment gets store-level HA instead of the image default of one shard replica per partition. The seed is delivered as -D system properties prepended into the PD JAVA_OPTS ahead of pd.javaOpts, preserving the start script's automatic heap sizing; it applies at first bootstrap only, after which PD metadata is authoritative, all documented together with the resulting initial partition count change. pd.antiAffinity and store.antiAffinity default to preferred so the chart schedules on clusters with fewer nodes than replicas; values-cluster.yaml keeps required for both, NOTES warns when PD quorum members may co-locate, and the README documents the upgrade implications. The Disaster Recovery documentation describes what current PD builds actually do: the scheduled patrol only marks silent stores Offline, shard reconciliation and tombstone processing run only via the manual /v1/task/patrolPartitions endpoint, and the pd.patrol-interval and store.max-down-time properties are bound but never read, which is why the chart does not expose them. Periodic leader balancing and recovery metrics are referenced as upstream feature requests. extraEnv now also rejects JAVA_OPTIONS for pd, store, and server, because the start scripts drop the chart-managed JAVA_OPTS entirely when it is set. Schema accepts numeric strings for the new keys, values-file integers at or above one million no longer fail as scientific notation, and CI asserts the rendered -D content, covers both shard-count validation messages, and kubeconforms the sharded render.
…#3137) Document that the creating Server is consistent at HTTP 200 after apache#3138, while cross-replica convergence and PD-owned creation remain open upstream.
Multi-replica Server pods must use one JWT signing key or Hubble login fails behind the Service; chart-manage or BYO via server.auth.tokenSecret.
When auth is enabled without existingSecret, create a stable release-admin Secret so operators are not forced to pre-create credentials.
Default installs get a chart-managed admin Secret; leave Hubble off so API-only clusters stay lean, and enable the UI with one flag when wanted.
Split server.auth into admin and token blocks so inline credentials (password/value) are distinct from Kubernetes Secret refs (existingSecret), matching the clearer values API for operators.
Let Server register a reachable URL with PD and optionally expose the PD client Service, so standalone Hubble can discover the cluster without in-cluster DNS. Fail closed when advertiseUrl is set without PD meta mode.
PD resolves its raft peer hostnames once at boot and freezes the result as the RPC allowlist. With podManagementPolicy: Parallel a PD can start before its peers A records are published, freeze a partial allowlist, and permanently block the elected leader, wedging a fresh install. Add a wait-for-pd-dns init container mirroring the store wait-for-pd gate: hold the pd container until every peer hostname resolves, bounded by pd.waitTimeoutSeconds (values: pd.waitEnabled escape hatch, waitImage, waitTimeoutSeconds, waitResources, schema-enforced). busybox nslookup ignores resolv.conf search domains, so the gate derives the cluster domain and retries the FQDN. The PD headless Service publishes not-ready addresses, so records appear at IP assignment and the gate cannot deadlock. First-boot mitigation only: a PD rescheduled with a new pod IP still requires the image-side allowlist refresh, noted under Limitations. Hardening and docs in the same pass: - explicit updateStrategy and persistentVolumeClaimRetentionPolicy (Retain/Retain; honored on K8s >=1.27) on the PD and Store StatefulSets, value-driven and schema-typed - checksum/auth pod annotation on the Server Deployment so rotating an existingSecret rolls Server pods on the next upgrade; hashes Secret names, keys, and resourceVersion, never Secret data. Best-effort: template-only pipelines render a constant, and the first upgrade after a fresh install rolls Server once - NOTES.txt no longer echoes the admin password - README: release-name assumption note, port-forward instead of exec curl in troubleshooting, Upgrading notes for the one-time PD and Server rollouts this version causes, and Limitations entries for the image-side PD allowlist and PD management REST issues (chart 0.1.1)
This PR introduces the chart, so there is no published 0.1.0 to bump from and the version should stay at the initial one until a release happens. Reword the Upgrading note for the same reason: it described rolls that happen when upgrading "from an earlier revision", which does not exist here. State the forward-looking behavior instead, that any Pod template change rolls that workload, and keep the PD allowlist caveat and the first-upgrade Server roll, both of which still apply.
The lint-and-render legacy guard caught a regression: pd.waitImage was added to the pd required list, so a release created before the field existed failed schema validation under --reuse-values. Make the field optional and default the image in the template instead, matching how waitTimeoutSeconds already degrades. Also flip the gate condition to "not explicitly disabled" so those same legacy releases pick the DNS gate up on upgrade rather than silently losing the first-boot protection because waitEnabled is absent from their values.
The guard rejected advertiseUrl unless auth was on or in-chart Hubble ran in pd mode, on the premise that only then does the chart register server.urls_to_pd. That premise stopped being true when the chart moved every Server to PD meta mode unconditionally: server-deployment.yaml sets $pdMeta := true, so urls_to_pd is always written. Rendering with advertiseUrl set, auth off and Hubble off now emits HG_SERVER_URLS_TO_PD with the configured URL, which the guard had been refusing to produce. The URL-format guard (absolute http:// or https:// only) is unchanged. Correct the README passage that documented the removed requirement.
PD resolves its raft peer allowlist once at boot, so under Kubernetes a peer whose Pod IP was unpublished at that moment, or changes later, is rejected: a first-boot race that could wedge an install, and a permanent rejection after any PD reschedule. The upstream raft.ip-whitelist.enabled switch fixes this at the source, leaving peer authentication to Kubernetes-level controls. Render -Draft.ip-whitelist.enabled=false into PD's derived JAVA_OPTS via the new pd.raftIpWhitelistEnabled value (default false, schema-typed; images predating the switch ignore the property), and remove the interim wait-for-pd-dns init container with its pd.wait* values, schema entries, and peer-hosts helper. The gate only ever mitigated the first-boot half of the problem. Verified on Kubernetes with a PD image carrying the switch: two deterministic installs, PD follower/leader crash, PD and Store majority loss, leader network partition, PVC reattach, rolling restart under write load, auth on every Server replica, a 0.1.2-to-this upgrade, Secret rotation, and a zero-override install followed through the documented first-run steps. Zero "Blocked connection" and zero "Could not resolve allowlist entry" lines across the entire campaign; the pod-IP recycle case that previously failed reproducibly now passes. Also update the chart CI's JAVA_OPTS assertions, which matched a quote-terminated shard-count prefix and therefore broke once the whitelist flag was appended, and bring the README in line: the Limitations and Upgrading sections described the removed init container, and the values table had no row for the new setting.
…ning Setting server.auth.token.value shorter than 32 bytes passed every chart check and then failed at runtime: all Server pods crash looped on the entrypoint assertion and helm install --wait timed out with no indication of the cause. Add a minLength to the schema so the value is rejected at render time with a precise path. The default values set no container resources, so each JVM sizes its heap against total node memory rather than a cgroup limit. On a multi-node cluster where several pods share a node the heaps oversubscribe it. Note the cluster preset in the README and warn from NOTES.txt. Images could only be referenced as repository:tag, so a release could not pin an immutable digest. Add an optional image.digest per component that takes precedence over the tag. Give the Helm test hook bounded default resources so it is admissible on quota-managed namespaces.
The chart shipped no unit tests. Cover the behaviour that fails silently: the PD raft whitelist flag and derived JAVA_OPTS, image digest pinning, the Server auth checksum annotation and Secret wiring, update strategy and PVC retention, quorum arithmetic at one three and five replicas, the test hook resource bounds, and every validateValues guard asserted against its real error message.
Record the values that had no documentation: the optional image.digest for each component, and the 32 byte minimum on server.auth.token.value that the schema now enforces. Add the credential lifecycle that operators hit first. The chart-managed auth Secret survives uninstall and is reused by a later install of the same release name, and helm template cannot read an existing Secret so the password it prints is only a render-time placeholder. Add a worked example for supplying an admin Secret instead of letting the chart generate one. Correct two stale statements: the Limitations note pointed to the Disaster Recovery section as being below when it is above, and the pinning advice said three component tags when there are four, since Hubble also tracks a mutable tag.
Picks up apache#3159 (321ba4d), the REST API adaptation for Hubble and the k8s-mode enhancements that the pending confirmation pass depends on, along with apache#3140, apache#3153, apache#3173, apache#3171, apache#3177, apache#3176, apache#3178 and apache#3149. No conflicts, and no change to helm/.
Every Server replica registered the shared client Service URL with PD, so the discovery registry collapsed three replicas into one logical entry and PD-discovered clients such as Hubble saw a single Server. The registration is a lease renewed by a per-instance heartbeat, so a replaced Pod's entry ages out on its own; announcing each Pod's own IP keeps the replica list truthful without leaving permanent stale entries. Empty server.advertiseUrl now announces the Pod IP through a POD_IP downward-API variable. Setting advertiseUrl keeps its meaning as the shared external endpoint for an outside Hubble. POD_IP joins the reserved environment names so extraEnv cannot shadow it. Ports c2d086d from helm-dev and adds a unit suite covering the fieldRef, the default URL shape, the advertiseUrl override and the reserved name.
…commands The Limitations entry said the PD management endpoints reject requests on current images, so the Disaster Recovery flow might be unavailable. Measuring it shows the opposite. PD compares the Basic-auth username against a fixed internal set and never reads the password, so any password, including an empty one, is accepted for those names. The endpoints are effectively unauthenticated rather than unusable, which is a caveat about exposure, not availability. That also makes the documented recovery commands wrong. Written without a credential they answer HTTP 200 with an Unauthorized body and the task never runs, so an operator mid-incident sees success and gets nothing. Add the credential the endpoints actually require and say why the password is empty. Also record that success, refusal and a missing credential all return HTTP 200, so the status code carries no signal for health checks.
…ment health vs ready PD's /v1/health answers 200 as soon as the REST listener is up and never consults raft, so every PD and Store probe and the Store init container's PD wait count listeners, not quorum members (apache#3183). The fix, apache#3185, adds /v1/ready from 1.8.0. - pd.readinessPath and store.waitPath, both defaulting to /v1/health, so the switch to /v1/ready is a values change made with the 1.8.0 pin; the schema rejects paths without a leading slash - README: Limitations entries for the liveness-only health endpoint and for the 45 second discovery lease (measured 30 to 35 seconds); the Store wait is described as a PD wait rather than a quorum wait; the Server now registers its Pod IP, not the Service URL - NOTES and the init container messages no longer claim a quorum - tests: pd_readiness_path_test.yaml, five cases
Points at apache#3185 and says the defaults flip with the 1.8.0 image pin, so the change is not lost once that PR merges.
PD images from 1.8.0 (apache#3189) check the Basic-auth password of every management call against auth.secret-key and refuse to start without one. The chart now keeps that value in a kept release-pd-auth Secret, or in pd.auth.existingSecret, and hands it to the three readers: PD as HG_PD_AUTH_SECRET_KEY, the Server storage wait as PD_AUTH_PASSWORD, and Hubble as operations.pd.password written into its properties file by the existing wrapper. A checksum/pd-auth annotation on the three Pod templates rolls them when the Secret changes; the Server annotations block is now rendered unconditionally for it. Priority and lookup semantics mirror server.auth.token. Older images ignore the password, so the wiring is harmless on the images the draft currently tracks. The values schema requires one of existingSecret, value or autoGenerate and refuses newlines, carriage returns and backslashes in an inline value, since it lands in a Java properties file; the template guard repeats the first rule for values that bypass the schema. The three chart-managed variables join the reserved extraEnv lists. README: Chart Details bullet, four parameter rows, Disaster Recovery calls carry the secret, and the Limitations bullet separates the 1.7.0 behaviour from 1.8.0. NOTES prints how to read the secret. New suite pd_auth_secret_test.yaml, 9 tests; 58 in total. Lint on three presets; renders 16 objects by default and 19 with Hubble. Measured on a kind cluster with images built from master plus apache#3185, apache#3187 and apache#3189: the Secret is created, PD starts with the variable, the Server storage wait passes with the credential, and Hubble lists all nine nodes.
Follow-ups from CodeRabbit's 2026-09-18 pass on #221: - Install helm-unittest by the v1.0.0 tag's commit SHA, so a moved tag cannot change what executes on the runner. - The recovery runbook ran two port-forwards and three curls as one foreground sequence, which blocks at the first forward. Note the second terminal, stop the Service forward before the leader forward, and move the inline comments to their own lines within 100 columns. - Exclude a leading form feed from inline admin passwords: the config reader trims it when the admin is first created, so the Secret and the effective password would silently differ.
Review fixes from #221: The helm test Pod never received .Values.imagePullSecrets, so on an authenticated private mirror the four workloads pull with credentials while helm test enters ImagePullBackOff. The hook now propagates the setting like the workloads do, with a unit case. PD and Store storage sizes live in the StatefulSet volumeClaimTemplates, which Kubernetes forbids changing, so an upgrade with a new size was rejected in full with nothing telling the operator the supported path. The parameter tables mark both sizes install-time and Upgrading gains the resize procedure: patch each PVC, wait, recreate the StatefulSet with --cascade=orphan, upgrade with the matching value.
…very Two gaps a review of the replies surfaced before posting them: The pd.auth.existingSecret row now states the same value constraint as the inline path (printable ASCII, no leading whitespace, no backslashes); an external Secret bypasses the schema, so the rule has to live in the contract text. The recovery runbook now re-reads /v1/members after the task sequence: if leadership moved mid-sequence, the later tasks ran on a follower and did nothing, so they are rerun on the new leader. Also strips trailing whitespace the workflow picked up in e62cac3.
25 tasks
3 tasks
Add server.readinessPath (default /versions, so nothing changes on current images), mirroring pd.readinessPath: values.yaml, the schema (pattern ^/) and the Server readinessProbe in server-deployment.yaml, plus a README parameter row and a two-case unit suite (74 total). On a Server image that serves GET /readiness (apache#3212, proposed in apache#3221), setting the value to /readiness makes a Server that cannot serve graph traffic answer 503 and drop out of the Service instead of returning 500 to every graph request. Startup and liveness stay on /versions so a Server that merely lost its storage is not restarted. Implements the change proposed and measured by @SebastianGruza in #229 (six fault scenarios at 1 Hz sampling, zero readiness transitions across Store and PD rolling restarts).
The Store commits well beyond its heap. conf/application.yml includes the pd Spring profile, and conf/application-pd.yml sets rocksdb.total_memory_size to 32000000000; RaftRocksdbOptions splits that into a RocksDB write cache and block cache, so both are bounded by 32 GB rather than by the container. The jraft log storage registers a further 1 GiB LRU cache once per process. Against -Xmx1024m -XX:MaxDirectMemorySize=512m the old 4Gi limit sat below the steady state: a k3s run OOM-killed all three Stores after about 1 GB of data and they stayed in CrashLoopBackOff, while 8Gi held at 4.42 GiB anonymous RSS. Request 5Gi, limit 8Gi, with the accounting recorded in the preset and in Limitations. The limit bounds the damage, not RocksDB. The Store entrypoint rebuilds SPRING_APPLICATION_JSON from its own variables and the chart mounts no config file, so rocksdb.total_memory_size cannot be set from the chart today; that is noted as an image-side gap. Reported with measurements by @SebastianGruza on #221.
Both auth checksums appended the live Secret resourceVersion unconditionally, including when the credential was an active inline value. That rolled the Pods a second time on the no-change upgrade after a rotation: the rotation render still saw the old resourceVersion, and the next render saw the new one with the same desired value. The revision input is now chosen per credential. An active inline value contributes its digest alone, so a rotation rolls once. An external or chart-generated Secret keeps the live resourceVersion, which is the only signal it has. Server admin and token select independently. Tests pin the exact annotation for four source combinations, so the parts list cannot change unnoticed; the existingSecret cases set an inline value too and prove it stays out. A render carries no live Secret, so the lookup half stays for the lifecycle run. Answers the open thread on #221.
The OnDelete procedure told the operator to wait for the replaced Store to show Up in PD. That is not a recovery barrier. StoreNodeService.register() persists StoreState.Up and only then notifies the Store, where HgStoreEngine.stateChanged starts restoreLocalPartitionEngine(); a failure there is logged and the state stays Up. The Store is Up before it has restored anything, so an operator following the old text could delete the next replica while the first was still rejoining. Replace it with the strongest check these images support: per shard group, the full shard count, exactly one leader, and the replaced Store back in the groups it holds, read from the PD leader through /v1/shardGroups. State the residue plainly. That is PD's membership record, not proof that the Store finished loading its partitions and caught up, and no endpoint reports restoration-complete, so the text asks for a margin, keeps the PDB floor as the backstop, and names the missing image-side signal. Answers the P1 recheck on #221.
HugeConfig reads the Server properties file through Configurations.properties(), whose reader trims the value, so a password with a trailing space or tab was stored padded in the Secret and applied to the first-created admin account trimmed. Authenticating with the value the Secret holds then fails, with nothing in the install to suggest why. The admin-password pattern already refused leading whitespace but accepted a trailing space, tab or form feed; pd.auth.value accepted a trailing space. Both now require the last character to be non-whitespace, and a single character value stays valid. An inner space is still allowed, because only the ends are trimmed. Schema aborts cannot be matched by the unit framework at v1.0.0, so the CI job gains three must-fail cases and one positive control for the inner space. Answers the P2 recheck on #221.
Two halves of the same mistake: the chart guarded PD and Store shrinks but let a PD increase through, and the documented Store drain named two tasks that cannot retire a healthy Store. PD. The rendered peer list reaches raft only as NodeOptions.setInitialConf, which jraft applies when a node bootstraps without its own configuration. On an initialized group a 3-to-5 upgrade adds Pods and leaves the voting configuration at three. Membership moves through RaftEngine.changePeerList, which the PD client API reaches and no REST route exposes, so the chart refuses the upgrade and says where the operation actually lives instead of offering a sequence nobody has run here. Store. patrolPartitions reallocates groups whose shard count is wrong and hands off groups belonging to Stores already in Tombstone; balancePartitions spreads shards over the active Stores, the leaving one included. Neither retires it, so the old completion condition could never arrive. The procedure now transitions the leaving Store to Tombstone, the same path the recovery runbook uses, checks the remaining Stores still satisfy the persisted replication factor, maps ordinals to Store ids, and waits on /v1/shardGroups before the StatefulSet shrinks. The guard needs a live StatefulSet, so a fresh install at five PD replicas still renders; that case is pinned in the topology suite. Answers two P2 rechecks on #221.
PD's /v1/health returns 200 as soon as Jetty is up and never consults raft. With three PDs that is the right liveness signal, because restarting a follower for a normal election would turn one election into a rolling outage. With one PD there is no election to lose: a PD that steps down and cannot recover, as after a failed raft snapshot on a full disk (apache#3222), keeps answering /v1/health while serving no writes, and liveness never restarts it. Add pd.livenessPath, empty by default and derived: /v1/health above one replica, /v1/ready at one. The startup probe follows the same path, because Kubernetes suppresses liveness until startup succeeds, so leaving startup on /v1/health would give a single PD only the 60 s liveness budget to reach raft readiness after a restart and a slow log replay would crash-loop; following liveness charges that window to the 300 s startup budget instead. Multi-PD renders are unchanged. The value stops being needed if PD starts answering 503 from /v1/health in this state. Reported with measurements by @SebastianGruza on #221.
The chart turns PD's raft IP whitelist off in-cluster and until now had no replacement, so any Pod in the cluster could reach PD, Store and Server. networkPolicy.enabled renders one policy per component (PD, Store, Server, and Hubble when enabled). Each policy isolates its own Pods in both directions and carries all of their allows, so no apply order leaves a Pod denied before its allows exist; there is no separate default-deny object. The allows follow the traffic the images make: PD peers on raft and gRPC (followers forward to the leader), Store and Server to PD gRPC and REST, Store peers on raft only, Server to Store gRPC and REST (wait-partition.sh), Hubble in pd mode to PD gRPC and REST and Store REST, Hubble and the test hook to the Server, and DNS on port 53 from every component. Nothing outside the release is admitted unless networkPolicy.<component>.extraIngress names it, and the schema requires every such rule to name its peers. Exposing PD, Server or Hubble (a NodePort or LoadBalancer Service, an Ingress, or server.advertiseUrl) with an empty extraIngress fails the render instead of opening the port; the check runs after the existing exposure checks. Hubble also gets extraEgress for its optional outside endpoints. A Hubble policy without extra rules omits the ingress key, which the API server would otherwise drop and Helm would rewrite on every upgrade. Off in values.yaml, which cannot know the release's clients; on in values-cluster.yaml. The validateValues refusal of networkPolicy.enabled is removed. The README notes that the Store image downloads libjemalloc.so from github.com on each start, which times out under the policies.
The Apache RAT check fails on the helm-unittest snapshot file added with the NetworkPolicy tests because it carries no license header. helm-unittest ignores the comment when it reads the snapshot and leaves the file as is on a passing run (checked with 1.0.0 and 1.1.2).
Brings in apache#3220 and apache#3157; no file overlaps with the chart branch.
A lifecycle run on a 3+3+3 kind install (chart d429dc4, :latest images) followed these procedures word for word and found three of them wrong or unusable as written. The OnDelete Store barrier could not see a Store that was down: PD keeps a Store Up and in every shard group until its keep-alive entry expires (300 s), so the documented check answered healthy on every sample through a 150 s replacement. The barrier now starts from the replaced Pod being Ready and a fresh lastHeartBeat, and points at the Store's own :8520/v1/partition/<groupId> for a closer look. It also named pd.partition.shardCount, which does not exist; the key is pd.partition.defaultShardCount, empty by default and derived as 3. Same name fixed in Scaling. Retiring a Store that was replaced with an empty PVC does not repair the cluster. The replacement keeps the Pod's raft address, so the reallocation adds a peer the group already has: 20 minutes and three patrols later all 12 groups still listed the retired id, the replacement held no partitions, and balancePartitions moved nothing. Nothing in /v1/stores, the cluster state or Hubble shows the lost redundancy. The section now says not to delete a Store PVC and what the failure looks like. The recovery runbook told operators to rerun the tasks on the new leader without mentioning that balancePartitions locks balanceLeaders for 180 s, which surfaces as a bare HTTP 500. It also gave no way to tell a real run from a no-op; the discriminating outputs are now written down. Two upgrade notes added from the same run: a Server that starts while PD is rolling can come up without its Gremlin binding and stays that way until the Pod is deleted (4 of 12 such starts), and dropping an inline credential back to the managed Secret rolls PD, Server and Hubble once more. Rotating the admin password through the Secret now says what actually happens, including the per-replica auth cache window measured at 9 to 21 minutes.
A retest of the procedures rewritten in 8ca50a2, run as written on a fresh 3+3+3 install with the release named hg, found five places where the text did not work. Workload and Service names were given as <release>-hugegraph-*, which is only true when the release name does not contain "hugegraph"; for the README's own release it produced names like hugegraph-hugegraph-store. The naming paragraph now defines <fullname>, and the three commands that used the wrong form use it. The Gremlin check answered 401 because it carried no credential, and it printed binary because the Server gzips the reply; it now reads the password the way Installing the Chart does and passes --compressed. Changing the admin password through the API left helm test passing or failing by replica while the auth caches expired. The README now says to restart the Server Pods, which applied it on every replica in 32 s. The Store's per-group view reports term and index 0 on a cluster with no writes, so the barrier text now says to compare them with a peer.
11 tasks
Reserve the chart-owned checksum/ podAnnotations prefix on every component: user annotations render after the chart's own and the last duplicate key wins, so a fixed value would pin the rotation checksum. Refuse server.securityContext.readOnlyRootFilesystem=true, mirroring the Hubble guard, because the Server wrapper rewrites rest-server.properties inside the image. Reject updateStrategy.rollingUpdate options together with type OnDelete in the schema for PD and Store; Kubernetes refuses that combination at apply time, which would otherwise surface mid-upgrade. Unit tests cover the template guards and the CI reject-invalid-values step covers the schema constraint. Use the get-with-default pattern for the $exposed NetworkPolicy check, matching the rest of validateValues. Remove the constant $pdMeta and $wrapper indirection from the Server Deployment, and remove the unused server.restServer.minFreeMemory / batchMaxWriteThreads knobs end to end (template, values, schema, README); the chart is unreleased, so nothing depends on them. helm template output on the default, single and cluster presets is byte-identical before and after. Refresh the docs: the server.testResources default in the README matches values.yaml again, and the runbook follows apache#3232, apache#3233 and apache#3234 (merged 2026-09-24). The empty-PVC Store retirement was re-proven on images built from master at dbb6663: all 12 groups converged onto the replacement 1 s after Tombstone and patrol, with 0 acknowledged writes lost, so the Disaster Recovery section documents the working procedure with a version caveat instead of a prohibition.
bitflicker64
marked this pull request as ready for review
September 24, 2026 15:30
imbajin
reviewed
Sep 24, 2026
imbajin
left a comment
Member
There was a problem hiding this comment.
Review at 8603cdb
Score: 8.6/10. Six independent lanes completed; no new P0/P1 chart blocker was verified. This is not a merge approval.
- Local validation passed 139 unit tests, three preset lints, positive/negative rendering, legacy-values rendering and packaging. The exact-head Helm CI job also passed. No new live-cluster run is claimed.
- The optional PD-mode Hubble credential issue is already tracked in org #221; raw existingSecret values must be escaped before writing properties. I added +1 instead of duplicating it here. The remaining boundary-whitespace case also belongs to its existing thread.
- Sync the paired documentation before publishing: Store Up alone is not a recovery gate, and the cluster preset does enable NetworkPolicy.
Complete the remaining checks and affected credential/documentation fixes. #3228 and #3229 remain separate runtime acceptance items for the 1.8.0 deployment story.
This was referenced Sep 24, 2026
Cut the README from 1,472 to 1,003 lines now that the docs site carries the operator walkthroughs. Each moved section keeps its load-bearing warning and commands and links its docs-site path: NetworkPolicy details, Cluster Health, Scheduling, Partition Sharding, the Disaster Recovery narrative (the keep-the-PVC rule and the retirement commands stay), the Scaling procedures, the outside-Hubble paths, and the Could-not-rebind measurements. The quickstart, presets, Kind flow, upgrade warnings with the OnDelete Store roll, the values tables, the validation list, every troubleshooting symptom and check command, and the Limitations stay. Deduplicate repeated passages to one home each: the ordinal-truncation explanation (Release Name Too Long), the anti-affinity trade (Installing), and the Hubble single-replica/H2 constraints (the Hubble section; the values.yaml comment now points there). README-only plus a values.yaml comment: helm template output is byte-identical, lint passes on the three presets, and all 139 unit tests pass. The docs-site links resolve once apache/hugegraph-doc#494 merges.
77 tasks
Close the acceptance gap the review demonstrated: helm test now resolves a new headless Server Service and requires an authenticated, graph-bound Gremlin answer from every Server Pod, and from at least the replica floor, retrying for 150 s so a transient PD election is not misread as a broken Pod. A Server that started during a PD roll passes readiness and serves REST while every Gremlin call fails, and the old Service-level checks accepted that state. No RBAC; probes untouched. CI runs the rendered hook against loopback fixtures where /versions and /graphs answer 200 while one Pod's /gremlin fails, and requires a nonzero exit. Make the production preset safe by default: values-cluster.yaml sets store.updateStrategy.type=OnDelete, documented as stopping automatic advancement only, not as a safety proof; the manual one-at-a-time procedure with its per-group checks still applies. Restrict updateStrategy.rollingUpdate.maxUnavailable to the integer 1: a larger value or percentage lets the controller disrupt two members of a three-member raft group or shard, and PDBs do not constrain controller rollouts. Derive the Server startup budgets from one conservative timeline: kubelet may run the first probe immediately, so the guaranteed alive time is (failureThreshold - 1) * periodSeconds; the floor becomes ceil(450/period) + 1 (default 91 x 5) and HG_SERVER_STARTUP_TIMEOUT_S is the guaranteed time minus the 300 s storage wait. Fail closed in the Hubble wrapper on a PD REST secret the properties parser would reinterpret (an existingSecret bypasses the schema), and refuse networkPolicy.enabled with an in-cluster pd-mode Hubble plus server.advertiseUrl unless networkPolicy.hubble.extraEgress names the advertised destination. Two bot findings were refuted and left unchanged: server.port in Hubble's properties is Hubble's own listener, and PD serves /v1/ready and /v1/health unauthenticated by design (the store.waitPath row now says the init container sends no credential). Verified on kind (3 PD + 3 Store + 3 Server, cluster preset, :latest built 2026-09-26): healthy helm test passes; a sequential PD roll with staggered Server restarts reproduced the broken-binding state on the second attempt, the new test failed naming the Pod, and passed again once that Pod was deleted; a Store template change under OnDelete moved the updateRevision without replacing any Pod. 153 unit tests, lint on three presets, and preset renders differ only by the headless Service, the hook script, failureThreshold 91, and the preset OnDelete.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of the PR
Add an official Helm installation path for the distributed HugeGraph HStore topology on Kubernetes. The chart packages the PD, Store and Server startup contract that otherwise has to be reconstructed by each operator, with authentication on by default and an optional Hubble UI.
This PR succeeds #3132 (closed, history and earlier review threads preserved there). Its head is the org branch
hugegraph:feat/hstore-helm-chart-3132, the same branch as hugegraph#221, so this PR and the org-side review stay on one branch and cannot drift. Review happens on hugegraph#221, where the GitHub review apps can run; ASF repository settings block their permissions here.Main Changes
helm/hugegraph/: chart 0.1.0 with PD and Store StatefulSets, a Server Deployment, an optional Hubble Deployment, per-component NetworkPolicy, three values presets (values.yaml,values-single.yaml,values-cluster.yaml), a fullvalues.schema.json, and 153 helm-unittest cases.init-storeand registers per-Pod with PD, the Store waits for a PD quorum on/v1/ready, PD readiness is quorum-aware while startup and liveness stay on/v1/health, andHG_SERVER_STARTUP_TIMEOUT_Sis derived from the startup probe budget.pd.auth) consumed by PD, the Server storage wait and Hubble; chart-managed admin and JWT Secrets with bring-your-ownexistingSecretpaths..github/workflows/helm-chart-ci.yml: lint, renders on all presets, the helm-unittest suites, the reject-invalid-values battery, kubeconform, a legacy--reuse-valuesfixture render, and packaging.README slimmed (
3996bb127). The chart README dropped from 1,472 to 1,003 lines against the docs-site pages: the NetworkPolicy details, Cluster Health, Scheduling, Partition Sharding, the Disaster Recovery narrative, the Scaling procedures, the outside-Hubble paths, and the Could-not-rebind measurements moved to the operations page, each leaving its load-bearing warning and commands plus a link, and the repeated passages (ordinal truncation, anti-affinity, Hubble H2) now have one home each. The docs-site links resolve once apache/hugegraph-doc#494 merges.Verifying these changes
master83ef9f3; 0 of 8,967 acknowledged writes lost.0e754b98c, answering the org-side architecture review):helm testnow requires a graph-bound Gremlin answer from every Server Pod through a new headless Service (no RBAC, probes untouched), the production preset holds Store rollouts on OnDelete,maxUnavailableaccepts only integer1, and the Server startup budgets derive from the conservative first-probe-at-zero timeline. Verified live on kind (cluster preset,:latestbuilt 2026-09-26): a sequential PD roll reproduced a Ready Server whose Gremlin could not rebind, the new test failed naming that Pod, and passed once it was deleted; a Store template change under OnDelete replaced no Pod.masteratdbb6663a(carries fix(store): keep GET /v1/partitions working on follower stores #3232, fix(pd): return PDException body from task/balanceLeaders #3233 and fix(store): rejoin a Store rebuilt empty at its old raft address #3234): the replacement Store registered under a new id, and after the documented Tombstone pluspatrolPartitionsretirement all 12 shard groups converged onto it in 1 s; the replaced Store served 12/12 partitions with its data resynced, a restart with the kept PVC came back clean, and a continuous writer lost 0 acknowledged writes. On pre-fix images the same scenario converged 0 of 12 groups after 25 minutes.Review history
The 2026-09-17 work queue from the org-side review is complete: the inline findings are fixed on this head, the helm-unittest suites run in CI, and the current-image lifecycle run is done (evidence above). The remaining unresolved inline threads were answered on 2026-09-24 with the commit that fixes them: the chart now reserves the
checksum/pod-annotation prefix, refusesserver.securityContext.readOnlyRootFilesystem=true(the wrapper rewrites config inside the image), and rejectsupdateStrategy.rollingUpdateoptions combined withOnDeletein the schema.Follow-ups:
appVersionand the four component image tags to the next HugeGraph release and switch pull policies toIfNotPresent. Blocked on a 1.8.x tag existing; the latest release is 1.7.0.Does this PR potentially affect the following parts?
helm/and one CI workflow; no Java changes)Documentation Status
Doc - TODODoc - DoneDoc - No NeedDocumentation files in this PR or paired hugegraph-doc PR:
helm/hugegraph/README.md, plus the paired site pages in apache/hugegraph-doc#494 (EN and CN, a deployment page and an operations page, written against8603cdbb3).